Add Sync Checker #13580
Merged
Add Sync Checker #13580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rkapka
reviewed
Feb 5, 2024
Comment on lines
+84
to
+86
| type SyncChecker struct { | ||
| Svc *Service | ||
| } |
There was a problem hiding this comment.
The checker having the full service as a dependency doesn't look right to me... It seems like the initial sync service already implements the SyncChecker interface, you can use it as the blockchain's service dependency. One service depending on another is more natural.
There was a problem hiding this comment.
The main reason you cant do it is because initial sync has a dependency on the blockchain service. It is the reason the checker has been implemented
potuz
previously approved these changes
Feb 5, 2024
Comment on lines
90
to
92
| if s.Svc == nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Can we add a warning here? Seems like this should never be desired at runtime.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
terencechain
approved these changes
Feb 6, 2024
prestonvanloon
approved these changes
Feb 6, 2024
rkapka
pushed a commit
that referenced
this pull request
Feb 6, 2024
* fix it * add it in * typo * fix tests * fix tests * export and add test * preston's review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Cleanup
What does this PR do? Why is it needed?
To allow the blockchain service to be aware of the current node's sync status, the sync checker object is shared between both services to allow this status to be determined and disseminated across them.
Which issues(s) does this PR fix?
N.A
Other notes for review